Multidimensional Views 🦏 #288

fuchsto · 2017-02-17T16:59:28Z

Addresses #192

For use cases of multi-dimensional views, see tests:

https://github.com/dash-project/dash/blob/feat-192-nviews/dash/test/view/NViewTest.cc
https://github.com/dash-project/dash/blob/feat-192-nviews/dash/test/view/ViewTest.cc

Current benchmark results:

gcc 5.4

     units,  elem/unit, iterations,   view.gups,   algo.gups,     speedup
         4,          4,    1000000,      1.8313,      0.4129,      4.4352
         4,         16,     100000,      1.7277,      0.3269,      5.2847
         4,         64,     100000,      1.8660,      0.3332,      5.5995
         4,        256,      10000,      2.1120,      0.3511,      6.0158
         4,       1024,      10000,      2.1328,      0.3505,      6.0849
         4,       4096,       1000,      2.1272,      0.3489,      6.0960
         4,      16384,       5000,      2.1349,      0.3502,      6.0962
         4,      65536,       1000,      2.1363,      0.3502,      6.1007
         4,     262144,        500,      2.1363,      0.3501,      6.1026

This PR also includes some meta-helpers for ranges and container types:

https://github.com/dash-project/dash/blob/feat-192-nviews/dash/include/dash/Meta.h

To test if a type can be used for container elements, use:

static_assert(
  dash::is_container_compatible<ElementType>::value,
  "Type not supported for DASH containers");

This is now used in all available container types.

…feat-191-lranges

…192-nviews

…feat-192-nviews Conflicts: dash/include/dash/View.h dash/include/dash/view/IndexSet.h dash/include/dash/view/Sub.h dash/test/TestBase.h

…feat-192-nviews

…192-nviews

…feat-192-nviews

…192-nviews

…eat-192-nviews

…192-nviews

fuchsto · 2017-03-16T07:37:40Z

@fmoessbauer @devreal @rkowalewski Bounce for review!

devreal

Many good changes in here! Just some comments inline.

devreal · 2017-03-16T08:26:49Z

build.dev.sh

-                        -DENABLE_DEV_COMPILER_WARNINGS=ON \
-                        -DENABLE_EXT_COMPILER_WARNINGS=ON \
+                        -DENABLE_DEV_COMPILER_WARNINGS=OFF \
+                        -DENABLE_EXT_COMPILER_WARNINGS=OFF \


Please leave warnings on by default on dev builds.

Ah, that was by accident.

devreal · 2017-03-16T08:32:59Z

dash/include/dash/Cartesian.h

  /// Number of dimensions of the cartesian space, initialized with 0's.
-  SizeType     _ndim    = NumDimensions;
+  SizeType     _rank             = NumDimensions;


Comment and real initialization diverge here. I guess initialization with 0 would be correct?

devreal · 2017-03-16T08:43:26Z

dash/include/dash/GlobRef.h

-  : _gptr(DART_GPTR_NULL) {
-  }
+  : _gptr(DART_GPTR_NULL)
+  { }


Why you no delete t'is? ;)

That's what the TODO comment refers to. There are situations where GloRef objects are temporarily uninitialized. Perhaps I can eliminate them.

Turns out I already did! deleted the default constructor.

devreal · 2017-03-16T08:56:57Z

dash/include/dash/iterator/GlobViewIter.h

+    class    Ref_ >
+  constexpr GlobViewIter(
+//  const GlobViewIter<nonconst_value_type, P_, GM_, Ptr_, Ref_> & other)
+    const GlobViewIter<T_, P_, GM_, Ptr_, Ref_> && other)


This should be const GlobViewIter<T_, P_, GM_, Ptr_, Ref_> & other for the copy c'tor

fmoessbauer · 2017-03-16T10:20:56Z

dash/examples/bench.10.summa/main.cpp

@@ -335,7 +335,7 @@ void perform_test(

  if (myid == 0) {
    if (iteration == 0) {
-      cout << "-- Pattern: " << pattern << endl
+      cout << "-- Pattern: " << dash::internal::typestr(pattern) << endl


Great, but it would be very useful to have some documentation on the dash::internals

Oh yes, will add Doxygen comments for the new features. Note that namespace internal is excluded from API docs for obvious reasons. Perhaps some helpers like typestr should be public instead.

fmoessbauer · 2017-03-16T10:28:02Z

dash/include/dash/internal/TypeInfo.h

+           typeid(T).name()
+         );
+}
+


Documentation! Otherwise we will re-invent this in a couple of month, as no one knows that this exists.

See my comment above. I moved dash::internal::typestr to dash::typestr in header dash/meta/TypeInfo.h, it's documented and part of the public API now.

fmoessbauer · 2017-03-16T10:31:03Z

dash/include/dash/iterator/GlobViewIter.h

+    class    Ref_ >
+  constexpr GlobViewIter(
+//  const GlobViewIter<nonconst_value_type, P_, GM_, Ptr_, Ref_> & other)
+    const GlobViewIter<T_, P_, GM_, Ptr_, Ref_> && other)


fmoessbauer · 2017-03-16T10:32:40Z

dash/include/dash/iterator/GlobViewIter.h

+             _globmem,
+             *_pattern,
+             g_idx
+           );


Combine it to an oneliner to enable RVO.

fmoessbauer · 2017-03-16T10:34:20Z

dash/include/dash/pattern/BlockPattern1D.h

@@ -799,7 +818,6 @@ class BlockPattern<1, Arrangement, IndexType>
    /// Global block index
    index_type g_block_index) const
  {
-#if 1


Why all these #if #else

The deactivated code sections are previous implementations before I rewrote them as constexpr. Left them as comments to help debugging in case my constexpr variant was broken. Will remove them.

fmoessbauer · 2017-03-16T10:36:54Z

dash/include/dash/pattern/TilePattern.h

@@ -1354,15 +1336,15 @@ class TilePattern
  /**
   * Cartesian arrangement of pattern blocks.
   */
-  const BlockSpec_t & blockspec() const
+  constexpr const BlockSpec_t & blockspec() const


I guess this can be made noexcept? Same for most of the other methods.

Possibly, but that's not the actual concern of this PR.

fmoessbauer · 2017-03-16T10:38:53Z

dash/include/dash/util/LocalityDomain.h

+  std::vector<iterator>                             _groups;
+  std::vector<std::string>                          _group_domain_tags;
+  /// Split domains in the team locality, one domain for every split group.
+  std::vector<self_t>                               _parts;


Why are these changes in this PR?

Serendipity.

fmoessbauer · 2017-03-16T10:42:13Z

dash/include/dash/view/IndexSet.h

+               0,                             // accumulate init
+               std::multiplies<index_type>()  // reduce op
+             );
+    */
    return (


This could be so beautiful in C++14 ;)

Also in CLisp <3

fmoessbauer · 2017-03-16T10:43:57Z

dash/scripts/bench/benchmark_runs.yml

@@ -0,0 +1,11 @@
+config:


Cool! Do we already have some regression testing provider?

I'm on it, currently works for a prototype on my machine.

fmoessbauer · 2017-03-16T10:45:26Z

dash/test/algorithm/AccumulateTest.h

-  virtual ~AccumulateTest() {
-    LOG_MESSAGE("<<< Closing test suite: AccumulateTest");
-  }
+    _dash_size(0)


IL not necessary as this is derived from TestBase

fuchsto · 2017-03-17T13:40:59Z

@devreal @fmoessbauer Your requested changes are addressed in my last commits. 🦏

fmoessbauer · 2017-03-17T15:31:54Z

This is a really really cool feature!!!

fuchsto · 2017-03-17T15:56:49Z

I'm quite satisfied for now, too.
The view expressions make some nasty use cases trivial to handle, like here:

https://github.com/dash-project/dash/blob/feat-192-nviews/dash/test/view/ViewTest.cc#L786

local(blocks(sub(array)))[1].begin()

It's horrible to express manually, even in pseudo code.

fuchsto added 30 commits February 9, 2017 21:19

Adding n-dim view expressions

0d7a76d

Merge branch 'feat-191-lranges' of github.com:dash-project/dash into …

1890e3f

…feat-191-lranges

Extending view expressions to n-dim

9ecac86

Merge branch 'development' of github.com:dash-project/dash into feat-…

a7e32a4

…192-nviews

Fixed broken code from merge in NViewMod

8f62258

Extending index sets to multiple dimensions

4f019fc

Extending index sets to multiple dimensions

35eb605

Extending index sets to multiple dimensions

53def95

Extending index sets to multiple dimensions

1ecc208

Extending index sets to multiple dimensions

df1fe2b

Reactivated Range tests

3ca7375

Extending index sets to multiple dimensions

4e02b1a

Extending index sets to multiple dimensions

aa10389

Extending index sets to multiple dimensions

8df97d5

Extending index sets to multiple dimensions

63d9a8c

Extending index sets to multiple dimensions

89a381c

Fixing performance (constexpr folding) of view types

e085c0e

Added notes on rvalue/lvalue ref binding

39dd797

Merge branch 'feat-191-lranges' of github.com:dash-project/dash into …

7b8353e

…feat-192-nviews Conflicts: dash/include/dash/View.h dash/include/dash/view/IndexSet.h dash/include/dash/view/Sub.h dash/test/TestBase.h

Fixed assertion in UniversalMemberTest

66344d1

Merge branch 'feat-191-lranges' of github.com:dash-project/dash into …

a917ff6

…feat-192-nviews

Merge branch 'development' of github.com:dash-project/dash into feat-…

e191d36

…192-nviews

Merge branch 'feat-191-lranges' of github.com:dash-project/dash into …

79486e3

…feat-192-nviews

Merge branch 'development' of github.com:dash-project/dash into feat-…

bf30ce8

…192-nviews

Merge branch 'development' of github.com:dash-project/dash into feat-…

e4eec0d

…192-nviews

Merge branch 'feat-192-nviews' of github.com:dash-project/dash into f…

7c7e1ac

…eat-192-nviews

Merge branch 'development' of github.com:dash-project/dash into feat-…

6225eee

…192-nviews

Fixed signature of dash::mmult, removed static_assert in dash::Matrix

beae4c1

Fixed signature of dash::mmult, removed static_assert in dash::Matrix

d48faf8

Fixed const-correctness in GlobPtr, GlobIter, GlobRef

123a617

fuchsto and others added 8 commits March 16, 2017 02:08

Fixing resolution of local view origins

2518009

Fixed resolution of local view origins

9b99717

Extended tests of 1-dim views

b22f077

Extended tests of 1-dim views

c353d9f

Fixed definitions of view types for clang

74b89fe

Fixed chained n-dim views

170a0d1

Cleanup in tests of n-dim views

ab4cb9e

Merge branch 'development' into feat-192-nviews

f01070e

fuchsto mentioned this pull request Mar 16, 2017

Multidimensional View- and Range Concepts #192

Open

fuchsto added 2 commits March 16, 2017 07:25

Minor fix 1-dim view test

ab23cd8

Added assertions in 1-dim view tests

03d7e5e

fuchsto changed the title ~~[WIP] Multidimensional Views~~ Multidimensional Views Mar 16, 2017

fuchsto requested review from fuerlinger, rkowalewski, fmoessbauer and devreal March 16, 2017 07:37

Extending tests of n-dim views

a5b9987

devreal reviewed Mar 16, 2017

View reviewed changes

fmoessbauer reviewed Mar 16, 2017

View reviewed changes

fuchsto added 2 commits March 17, 2017 14:25

Cleanup and requested changes from reviews

d5bfcd3

Removed deactivated code snippets

5b3dca4

fuchsto changed the title ~~Multidimensional Views~~ Multidimensional Views 🦏 Mar 17, 2017

fuchsto changed the title ~~Multidimensional Views 🦏~~ Multidimensional Views 🦏 Mar 17, 2017

More cleanup, GlobRef now with correct reference semantics

c52cfcf

fmoessbauer approved these changes Mar 17, 2017

View reviewed changes

fuchsto merged commit a319e7b into development Mar 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multidimensional Views 🦏 #288

Multidimensional Views 🦏 #288

fuchsto commented Feb 17, 2017 •

edited

Loading

fuchsto commented Mar 16, 2017

devreal left a comment

devreal Mar 16, 2017

fuchsto Mar 17, 2017

devreal Mar 16, 2017

devreal Mar 16, 2017

fuchsto Mar 17, 2017

fuchsto Mar 17, 2017

devreal Mar 16, 2017

fmoessbauer Mar 16, 2017

fmoessbauer Mar 16, 2017

fuchsto Mar 17, 2017

fmoessbauer Mar 16, 2017

fuchsto Mar 17, 2017

fmoessbauer Mar 16, 2017

fmoessbauer Mar 16, 2017

fmoessbauer Mar 16, 2017

fuchsto Mar 17, 2017

fmoessbauer Mar 16, 2017

fuchsto Mar 17, 2017

fmoessbauer Mar 16, 2017

fuchsto Mar 17, 2017

fmoessbauer Mar 16, 2017

fuchsto Mar 17, 2017

fmoessbauer Mar 16, 2017

fuchsto Mar 17, 2017

fmoessbauer Mar 16, 2017

fuchsto commented Mar 17, 2017

fmoessbauer commented Mar 17, 2017

fuchsto commented Mar 17, 2017

Multidimensional Views 🦏 #288

Multidimensional Views 🦏 #288

Conversation

fuchsto commented Feb 17, 2017 • edited Loading

fuchsto commented Mar 16, 2017

devreal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuchsto commented Mar 17, 2017

fmoessbauer commented Mar 17, 2017

fuchsto commented Mar 17, 2017

fuchsto commented Feb 17, 2017 •

edited

Loading